- 
                Notifications
    
You must be signed in to change notification settings  - Fork 458
 
feat(multiagent): add FunctionNode to improve DX for deterministic cases #991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with the devex of this approach, it is very clean. As for a abstract class, I think we can migrate to that if customers want it in the future, but good with this for now.
| logger.debug("function_name=<%s> | executing function", self.name) | ||
| 
               | 
          ||
| start_time = time.time() | ||
| span = self.tracer.start_multiagent_span(task, "function_node") | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should be using this to record a function node:
sdk-python/src/strands/telemetry/tracer.py
Line 622 in 776fd93
| def start_multiagent_span( | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? We use it in both swarm and graph with the name of the node?
https://github.com/search?q=repo%3Astrands-agents%2Fsdk-python%20start_multiagent_span&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I flagged this because we are setting a field in this function named gen_ai.agent.name, and this is almost always not an agent. We should probably revisit how we do this in the other multi-agent cases, but not a blocker for this pr
sdk-python/src/strands/telemetry/tracer.py
Line 662 in 26862e4
| "gen_ai.agent.name": instance, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry misunderstood the callout.
Looking at it now, honeslty use of gen_ai.agent.name seems inappropriate for graph and swarm too at the top level. It seems like these should all be using gen_ai.operation.name.
So I guess we can
- Keep gen_ai.agent.name for consistency
 - Remove entirely until we need to add back
 - introduce a start_multiagent_function_node_span
 
I'm leaning towards 1 for consistency, seems like you are too if I'm not mistaken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets not block this pr, im fine with keeping it consistent for now. But lets create an issue to track the proper fix of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
        4b9b2f5
      
    c4b0610    to
    4b9b2f5      
    Compare
  
    
          Codecov Report❌ Patch coverage is  
 📢 Thoughts on this report? Let us know!  | 
    
| 
               | 
          ||
| def __call__( | ||
| self, task: str | list[ContentBlock], invocation_state: dict[str, Any] | None = None, **kwargs: Any | ||
| ) -> str | list[ContentBlock] | Message: | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any way we can provide more flexibility here? This essentially gets them to write MultiAgentBase. I'd rather they return string or something serializable, and we adjust accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can return a string though if they want? I added the | list[ContentBlock] | Message because I thought that was more flexible if you want to do more advanced things
| """ | ||
| self.func = func | ||
| self.name = name | ||
| self.tracer = get_tracer() | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO for later probably: streaming callables 😅 #961
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point let me take a look for the streaming
Description
In our docs we call out a Custom Node Type which can be used to implement deterministic nodes. But this requires customers to know about stop_reason and construct the DEEPLY nested return type of MultiAgentResult which uses NodeResult which uses AgentResult - this is too in the weeds in my opinion.
To improve developer experience, we will vend a FunctionNode which handles the hard part.
new DX
The alternative here is to vend an abstract class. I am on the fence about this. An abstract class feels more "correct" but the DX of simply providing the method feels easiest to me. Very open to discussing.
Related Issues
N/A
Documentation PR
pending
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.